Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add redirect test workflow / github action #3786

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

add redirect test workflow / github action #3786

wants to merge 23 commits into from

Conversation

simontaurus
Copy link

Currently there are no or only limited pre-deployment testing options (see also #1629).

This pullrequest adds a workflow that can be run in the main repo, any fork or on local checkouts if docker is available.

Test specifications are currently extracted from .htaccess file comments, e. g.

##TESTv1 '/mypath --header "Accept: text/html"' "https://my-target-domain.com/test.html"

Example: 4892875

Contributors can run the github action in their forks selecting only the subpath of interest:
grafik
Example: https://github.com/OpenSemanticWorld/w3id.org/actions/workflows/redirect_tests.yml

Run will fail if at least one test is not successful, log will show errors e.g.

Processing file /usr/local/apache2/htdocs/emmo/.htaccess
TEST '/emmo/domain-test --header "Accept: text/html"' => 'https://emmo-repo.github.io/domain-test/test.html'
ERROR  : Result is 'https://raw.githubusercontent.com/emmo-repo/EMMO/domain-test.ttl'
TEST '/emmo/domain-test --header "Accept: application/rdf\+xml"' => 'https://emmo-repo.github.io/domain-test/test.ttl'
ERROR  : Result is 'https://raw.githubusercontent.com/emmo-repo/EMMO/domain-test.ttl'
Error: Process completed with exit code 1.

Example: https://github.com/OpenSemanticWorld/w3id.org/actions/runs/7224639650/job/19686349546

More details can be found here: https://github.com/OpenSemanticWorld/w3id.org/blob/master/.test/README.md

Let me know if that makes sense to you and if I should stash my commits

@simontaurus simontaurus mentioned this pull request Dec 15, 2023
Open
TallTed
TallTed reviewed Dec 15, 2023
View reviewed changes
.test/README.md Outdated Show resolved Hide resolved
jesper-friis
jesper-friis reviewed Dec 15, 2023
View reviewed changes
.test/README.md Outdated Show resolved Hide resolved
@davidlehn
Copy link
Collaborator

This is an interesting testing approach. Thanks. We'll probably need to think this over a bit.

  • Probably could slim down that apache config since it's mostly comments. I can check our production config to line things up so it's doing realistic testing.
  • I didn't know workflows could have inputs like that.
  • Based on the old travis-ci linkchecker experiment, we don't want to default to running all the tests in every dir. As this service grows, so do bitrotten configs and links. Need to avoid other peoples broken setups causing false positive failures for a PR.
  • For a long while, I've had ideas for other checks that should also be run. For that I wanted to do something to look at the PR changed files and only run tests in those dirs (or whatever makes sense). So if you you edit files in /foo and /bar in a commit, it would only check those dirs. Seems like something similar could be applied here. I'm not sure the implementation details of that.
  • Ideally we'd run this sort of thing by default. To avoid wasting cycles, probably would want a pre-action that quickly checks what tests should even run. No reason to spin up docker and apache if no tests exist.
  • For my other project ideas, I was considering a .w3id.json (or .yaml, .etcetc) config. Do you think it would be better to have test pragmas inline in .htaccess? Or would a more easily parsable file with test info be ok? I can see pros/cons there.

simontaurus and others added 2 commits December 16, 2023 03:33
@simontaurus @jesper-friis
docs: language
0b241bf 
Co-authored-by: Jesper Friis <jesper-friis@users.noreply.github.com>
@simontaurus @TallTed
docs: language
acc919c 
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
simontaurus
simontaurus commented Dec 16, 2023
View reviewed changes
.test/conf/httpd.conf
#LoadModule speling_module modules/mod_speling.so
#LoadModule userdir_module modules/mod_userdir.so
LoadModule alias_module modules/mod_alias.so
LoadModule rewrite_module modules/mod_rewrite.so
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diffs to the default config: Rewrite modul must be enabled

simontaurus
simontaurus commented Dec 16, 2023
View reviewed changes
.test/conf/httpd.conf
# It can be "All", "None", or any combination of the keywords:
# AllowOverride FileInfo AuthConfig Limit
#
AllowOverride All
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diffs to the default config: AllowOverride must be enabled in the target directory

@simontaurus
Copy link
Author

simontaurus commented Dec 16, 2023 •
edited
Loading

This is an interesting testing approach. Thanks. We'll probably need to think this over a bit.
...

  • I agree it would be best to use the actual config. The current file is the default apache config with two changes applied (see comments)
  • have to admit I also just found out =)
  • I was keeping that in mind with the search path option
  • the workflow now checks only changed dirs on pull requests, example: Redirect tests opensemanticworld/w3id.org#9
  • good point. I've added a precheck before the docker steps, example: https://github.com/OpenSemanticWorld/w3id.org/actions/runs/7229780356/job/19701078363
  • Maybe we can support both (need to agree on the formats of course). My idea was that it's most convenient to write the tests directly above the rules that should cover the cases. This also provides additional documentation since the tests show what we actually want to achieve.

@davidlehn davidlehn mentioned this pull request Dec 23, 2023
Open
@simontaurus simontaurus mentioned this pull request Jan 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TallTed TallTed TallTed left review comments

@jesper-friis jesper-friis jesper-friis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simontaurus @davidlehn @jesper-friis @TallTed